Skip to content

fix(security): escape codegen strings and redact diagnostic body#930

Merged
jackwener merged 1 commit intomainfrom
fix/codegen-and-redaction
Apr 10, 2026
Merged

fix(security): escape codegen strings and redact diagnostic body#930
jackwener merged 1 commit intomainfrom
fix/codegen-and-redaction

Conversation

@jackwener
Copy link
Copy Markdown
Owner

Summary

Fixes two bugs identified during post-migration first-principles review:

  • candidateToJs string injection: site, name, domain, and arg name/type fields were written into generated JS adapter files without escaping single quotes. If any of these contained ', the generated adapter would have a syntax error. Now all string literals in codegen use .replace(/'/g, "\\'")
  • diagnostic body data leak: redactNetworkRequest truncated the body field but did not pass it through redactText(), meaning JWTs or bearer tokens in response bodies could leak into AI repair context. responseBody and responsePreview already used sanitizeCapturedValueredactText, but body was missed

Test plan

  • src/generate-verified.test.ts — 28 tests passed
  • src/diagnostic.test.ts — 15 tests passed

1. candidateToJs: escape single quotes in site, name, domain, and arg
   name/type fields to prevent syntax errors in generated JS adapters.
   Previously only description and help fields were escaped.

2. diagnostic: pass network request body through redactText() to
   prevent sensitive data (JWT, bearer tokens) from leaking into
   repair context. responseBody/responsePreview already used
   sanitizeCapturedValue which calls redactText, but the body field
   only had truncation.
@jackwener jackwener merged commit 714df64 into main Apr 10, 2026
9 of 11 checks passed
@hiSandog
Copy link
Copy Markdown
Contributor

Reviewed the diff against local code. The fix correctly addresses both issues:

  1. codegen string escaping in candidateToJs: The patch adds .replace(/'/g, "\\'") to site, name, domain, name (arg), and type (arg) fields. Verified these were the only unescaped string interpolations in the function — description and help already had escaping.

  2. diagnostic body redaction: Wrapping truncate() with redactText() in redactNetworkRequest closes the gap where body bypassed the JWT/bearer token sanitization that responseBody and responsePreview already get via sanitizeCapturedValue.

One minor suggestion: consider adding a unit test that feeds a candidate with single quotes in site/name/arg name to candidateToJs and asserts the generated JS is syntactically valid (e.g., via new Function() or acorn.parse()). The PR mentions tests pass but doesn't show an explicit injection test case for these newly-escaped fields.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants